packer_test: make packer test suite modular #13041

lbajolet-hashicorp · 2024-06-12T20:07:44Z

Having only one test suite for the whole of Packer makes it harder to segregate between test types, and makes for a longer runtime as no tests run in parallel by default.

This commit splits the packer_test suite into several components in order to make extension easier.

First we have lib: this package embeds the core for running Packer test suites. This ships facilities to build your own test suite for Packer core, and exposes convenience methods and structures for building plugins, packer core, and use it to run a test suite in a temporary directory.

Then we have two separate test suites: one for plugins, and one for core itself, the latter of which does not depend on plugins being compiled at all.

This sets the stage for more specialised test suites in the future, each of which can run in parallel on different parts of the code.

nywilken · 2024-06-13T16:27:54Z

packer_test/lib/base.go

@@ -1,4 +1,4 @@
-package packer_test
+package lib


I have to dive into these changes further. But I hesitate to treat this as a separate package. I changed the name to packer_test, as that is the Go idiom for denoting that this is a test package, which does not have direct access to anything in the packer package and also to ensure that the package is excluded from the final binary.

Previously, the go files ended with _test. go, which are excluded from the binary. It looks like this refactor changes that.

I would think the code will not be part of the final binary still, as it's not transitively imported from main.go, that said if you think that the package name is a problem, we can still move the lib files back to the upper package so that packages using the test suite will still include something called _test.

nywilken

I'm still working through this but my initial review leaves me with more questions than answers. What would you say is the benefit to the refactor?

nywilken · 2024-07-25T19:06:19Z

packer_test/core_tests/local_eval_test.go

@@ -1,8 +1,12 @@
-package packer_test
+package core_test


Why core_test? What information does it provide that packer_test may not?

nywilken · 2024-07-25T19:09:52Z

packer_test/core_tests/suite_test.go

+type PackerCoreTestSuite struct {
+	*lib.PackerTestSuite
+}
+
+func Test_PackerPluginSuite(t *testing.T) {
+	baseSuite, cleanup := lib.PackerCoreSuite(t)


I don't understand the need for PackerCoreTestSuite over lib.PackerTestSuite and how they relate or differ from lib.PackerCoreSuite(). Is this level of abstraction offering a benefit that may not be immediately noticeable?

So the difference between the two is mostly that the tests run are not the same, being two separate packages/instances of TestSuite means that both can run in parallel. They also may not have the same needs for plugins for example, so one will have to compile a bunch of plugins, and others don't.
As for the names right now, I picked something somewhat randomly, I'm completely open to rename them to something else if you think it's confusing, no problems here!

nywilken · 2024-07-25T19:27:01Z

packer_test/plugin_tests/init_test.go

@@ -1,6 +1,8 @@
-package packer_test
+package plugin_tests


I see now why you chose core_test above. This feels like unneeded package nesting to split the tests. These are all Packer command tests. In fact it appears that this package offers nothing new besides renaming PackerTestSuite to PackerPluginTestSuite. Please let me know if I am missing something?

That said, the tests for plugins should be renamed to better indicate what is being tested. For example TestPackerInitForce could be renamed to TestPackerInitWithForceFlag, which read on the screen would be

TestPackerInitWithForceFlag/installs_any_missing_plugins or TestPackerInitWithForceFlag/reinstalls_plugins_matching_version_constraints

Ah yeah I agree, naming could definitely be improved here, as we add more tests we'll need to figure out some naming scheme to make things clearer.

nywilken · 2024-07-25T19:29:57Z

packer_test/core_tests/local_eval_test.go

-				Assert(MustFail(),
-					Grep("Duplicate local definition"),
-					Grep("Local variable \"test\" is defined twice"))
+				Assert(lib.MustFail(),


My first question here is what is lib? Why not just MustFail(). The generic name lib gives no information on the relation of MustFail() to the test suite.

What do you think if we renamed Assert to RunAndAssert to make it clear that Assert runs the test suite and succeeds if the asserts are true?

RunAndAssert( MustFail(), Grep("Duplicate local definition"), Grep("Local variable \"test\" is defined twice"))

lib is the common parts, so the base TestSuite that you can extend in other packages, and the gadgets for testing, so all the checkers/pipe infrastructure.

We can name them common too if you prefer? I went with lib because I like the conciseness, but I don't mind renaming this.

As for renaming Assert to RunAndAssert, I agree it's clearer, but it's also longer, I like terse function names :p
But I'm not fundamentally against the renaming tbh

nywilken · 2024-07-25T19:52:30Z

Then we have two separate test suites: one for plugins, and one for core itself, the latter of which does not depend on plugins being compiled at all.

With the proper naming conventions I think we could run tests in using the run filter on go test command. Is dynamic plugin generation an issue or are you concerned that over time things will get slower?

lbajolet-hashicorp

Overall I'd say the two main benefits are extensibility/separation by theme and ability to run test suits in parallel. There's also some organisation concern here, as one directory to rule them all is not necessarily ideal. That being said, maybe it's premature and we can revisit this later, if/when the organisation of the package becomes a problem.

As for running tests with -run, yes it is indeed possible; but by default if the test suites are in the same package, they won't run in Parallel I would think. Though maybe it would be feasible with something like t.Parallel? I'd need to explore that, but then if we keep everything in one big package, I'm worried at some point we'll have a ton of files in one giga directory, which is not necessarily ideal imo.

lbajolet-hashicorp · 2024-07-25T19:56:18Z

packer_test/plugin_tests/init_test.go

@@ -1,6 +1,8 @@
-package packer_test
+package plugin_tests


Ah yeah I agree, naming could definitely be improved here, as we add more tests we'll need to figure out some naming scheme to make things clearer.

lbajolet-hashicorp · 2024-07-25T19:57:18Z

packer_test/core_tests/local_eval_test.go

-				Assert(MustFail(),
-					Grep("Duplicate local definition"),
-					Grep("Local variable \"test\" is defined twice"))
+				Assert(lib.MustFail(),


lib is the common parts, so the base TestSuite that you can extend in other packages, and the gadgets for testing, so all the checkers/pipe infrastructure.

lbajolet-hashicorp · 2024-07-25T19:57:55Z

packer_test/core_tests/local_eval_test.go

-				Assert(MustFail(),
-					Grep("Duplicate local definition"),
-					Grep("Local variable \"test\" is defined twice"))
+				Assert(lib.MustFail(),


We can name them common too if you prefer? I went with lib because I like the conciseness, but I don't mind renaming this.

lbajolet-hashicorp · 2024-07-25T19:59:01Z

packer_test/core_tests/local_eval_test.go

-				Assert(MustFail(),
-					Grep("Duplicate local definition"),
-					Grep("Local variable \"test\" is defined twice"))
+				Assert(lib.MustFail(),


As for renaming Assert to RunAndAssert, I agree it's clearer, but it's also longer, I like terse function names :p
But I'm not fundamentally against the renaming tbh

lbajolet-hashicorp · 2024-07-25T20:01:26Z

packer_test/core_tests/suite_test.go

+type PackerCoreTestSuite struct {
+	*lib.PackerTestSuite
+}
+
+func Test_PackerPluginSuite(t *testing.T) {
+	baseSuite, cleanup := lib.PackerCoreSuite(t)


So the difference between the two is mostly that the tests run are not the same, being two separate packages/instances of TestSuite means that both can run in parallel. They also may not have the same needs for plugins for example, so one will have to compile a bunch of plugins, and others don't.
As for the names right now, I picked something somewhat randomly, I'm completely open to rename them to something else if you think it's confusing, no problems here!

nywilken

I noticed that some of the quick name changes I mentioned haven't been applied yet. However, I think those were mostly minor details since this is additive code. If everything is working well, especially with the new gob refactor, let's go ahead and move forward with it, and we can make adjustments as we continue refining it.

While I don't have specific replacement names at the moment, I wanted to mention that the current names aren't immediately clear to me.

Having only one test suite for the whole of Packer makes it harder to segregate between test types, and makes for a longer runtime as no tests run in parallel by default. This commit splits the packer_test suite into several components in order to make extension easier. First we have `lib`: this package embeds the core for running Packer test suites. This ships facilities to build your own test suite for Packer core, and exposes convenience methods and structures for building plugins, packer core, and use it to run a test suite in a temporary directory. Then we have two separate test suites: one for plugins, and one for core itself, the latter of which does not depend on plugins being compiled at all. This sets the stage for more specialised test suites in the future, each of which can run in parallel on different parts of the code.

The compiledPlugins map used to be a global variable, which can be problematic as we move to independent test suites, since those test suites run in the same process space, the global variable could point to now deleted plugin versions/paths in separate suites, which would make tests fail with random errors. To avoid this, the map is now scoped to the test suite, and a new copy is created lazily if used by the test suite.

In terms of organisation, we keep functions that interact with plugins into its own file, therefore the function that build plugin versions should really be in plugin.go, not in suite.go.

As a small reorganisation attempt, this commit moves some file-system oriented functions from plugin.go into its own file so they are more logically grouped.

When testing a packer build or validate, one common use case is to check which plugins are loaded and used by Packer to run a command. This is generally done through Grep, but repeating the same pattern can be redundant, and if the output changes, all those need to be updated. Therefore, this commit introduces a PluginsUsed gadget, which can be orchestrated to ensure a plugin is used, or not used, allowing to check for multiple plugins at once.

github-actions · 2024-09-14T02:12:05Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

lbajolet-hashicorp added enhancement tech-debt Issues and pull requests related to addressing technical debt or improving the codebase labels Jun 12, 2024

lbajolet-hashicorp requested a review from a team as a code owner June 12, 2024 20:07

lbajolet-hashicorp force-pushed the split_packer_test_suites branch from 1454020 to e7ab35c Compare June 12, 2024 20:08

lbajolet-hashicorp changed the base branch from main to fix_locals_eval_order June 12, 2024 20:08

lbajolet-hashicorp force-pushed the fix_locals_eval_order branch from 69615d8 to 11815a1 Compare June 12, 2024 20:09

lbajolet-hashicorp force-pushed the split_packer_test_suites branch from e7ab35c to d86199e Compare June 12, 2024 20:25

lbajolet-hashicorp mentioned this pull request Jun 12, 2024

hcl2template: recursively evaluate local variables #13039

Merged

lbajolet-hashicorp changed the base branch from fix_locals_eval_order to main June 13, 2024 14:51

lbajolet-hashicorp marked this pull request as draft June 13, 2024 14:57

nywilken reviewed Jun 13, 2024

View reviewed changes

lbajolet-hashicorp force-pushed the split_packer_test_suites branch from 3b62757 to 90d99b0 Compare June 14, 2024 13:38

lbajolet-hashicorp changed the base branch from main to fix_locals_eval_order June 14, 2024 13:38

lbajolet-hashicorp force-pushed the split_packer_test_suites branch from 90d99b0 to 2e91694 Compare June 14, 2024 13:58

lbajolet-hashicorp force-pushed the fix_locals_eval_order branch 3 times, most recently from 1fcfafa to 7823d15 Compare June 17, 2024 16:33

Base automatically changed from fix_locals_eval_order to main June 17, 2024 20:52

lbajolet-hashicorp force-pushed the split_packer_test_suites branch from 2e91694 to e24d4f0 Compare June 17, 2024 20:56

lbajolet-hashicorp marked this pull request as ready for review June 17, 2024 20:56

lbajolet-hashicorp changed the title ~~Split packer test suites~~ packer_test: make packer test suite modular Jun 17, 2024

lbajolet-hashicorp force-pushed the split_packer_test_suites branch from e24d4f0 to bc8c395 Compare June 17, 2024 20:58

nywilken reviewed Jul 25, 2024

View reviewed changes

lbajolet-hashicorp commented Jul 25, 2024

View reviewed changes

lbajolet-hashicorp force-pushed the split_packer_test_suites branch from bc8c395 to db95a73 Compare August 6, 2024 15:13

nywilken approved these changes Aug 12, 2024

View reviewed changes

lbajolet-hashicorp added 3 commits August 13, 2024 14:43

packer_test: move BuildSimplePlugin to plugin.go

6507866

In terms of organisation, we keep functions that interact with plugins into its own file, therefore the function that build plugin versions should really be in plugin.go, not in suite.go.

lbajolet-hashicorp added 2 commits August 13, 2024 14:43

packer_test: move some plugin functions to fs.go

3afc5d3

As a small reorganisation attempt, this commit moves some file-system oriented functions from plugin.go into its own file so they are more logically grouped.

lbajolet-hashicorp force-pushed the split_packer_test_suites branch from db95a73 to 47ea17b Compare August 13, 2024 18:43

lbajolet-hashicorp merged commit ded0500 into main Aug 13, 2024
11 checks passed

lbajolet-hashicorp deleted the split_packer_test_suites branch August 13, 2024 18:54

github-actions bot locked as resolved and limited conversation to collaborators Sep 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

packer_test: make packer test suite modular #13041

packer_test: make packer test suite modular #13041

lbajolet-hashicorp commented Jun 12, 2024 •

edited

Loading

nywilken Jun 13, 2024

lbajolet-hashicorp Jun 13, 2024

nywilken left a comment

nywilken Jul 25, 2024

nywilken Jul 25, 2024

lbajolet-hashicorp Jul 25, 2024

nywilken Jul 25, 2024

lbajolet-hashicorp Jul 25, 2024

nywilken Jul 25, 2024

lbajolet-hashicorp Jul 25, 2024

lbajolet-hashicorp Jul 25, 2024

lbajolet-hashicorp Jul 25, 2024

nywilken commented Jul 25, 2024

lbajolet-hashicorp left a comment

lbajolet-hashicorp Jul 25, 2024

lbajolet-hashicorp Jul 25, 2024

lbajolet-hashicorp Jul 25, 2024

lbajolet-hashicorp Jul 25, 2024

lbajolet-hashicorp Jul 25, 2024

nywilken left a comment

github-actions bot commented Sep 14, 2024

packer_test: make packer test suite modular #13041

packer_test: make packer test suite modular #13041

Conversation

lbajolet-hashicorp commented Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nywilken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nywilken commented Jul 25, 2024

lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nywilken left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 14, 2024

lbajolet-hashicorp commented Jun 12, 2024 •

edited

Loading